Skip to content
This repository has been archived by the owner on Jul 21, 2020. It is now read-only.

Fix bug related to spaces in the project path #149

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix bug related to spaces in the project path #149

wants to merge 2 commits into from

Conversation

beescuit
Copy link

Closes #141

@msftclas
Copy link

msftclas commented Oct 10, 2018

CLA assistant check
All CLA requirements met.

@connor4312
Copy link
Contributor

Thanks for the contribution! If you don't mind, could you actually grab a package like shell quote and stick that in there? It's more of an edge case, but this would also break if someone used quotation marks in their path; shell-quote should fix it permanently :)

@beescuit
Copy link
Author

Oh, you're right. I didn't think about that possibility. Also, (sorry I'm still getting used to these commit standards...) how should the title/body of the commit be?

@connor4312
Copy link
Contributor

The commit you have looks good. Don't worry about it too much though, we can squash merge and fix it up.

@beescuit
Copy link
Author

beescuit commented Oct 10, 2018

Just pushed the commit using shell-quote. I still think it has to have some improvements (maybe implementing a function to escape the path in the script itself?) because apparently webpack's argument parser only accepts double quotes instead of single ones (shell-quote's default) at the start/end of the parameter, so I had to remove both the first and last characters and replace them with double quotes.
Taken from the module code: character.replace(/([#!"$&'()*,:;<=>?@\[\\\]^`{|}])/g, '\\$1') inside a map should escape all special characters. (and then it would add double quotes to the start and end of the string)
Tell me what you think is best.

@beescuit
Copy link
Author

beescuit commented Oct 10, 2018

Whoops. The current code won't work for non-spaced paths (lol). I'm working on it to make it run. (I'm probably going to implement the escape function inside the script instead of using external modules)

@connor4312
Copy link
Contributor

I would recommend against that, shell escaping is surprisingly hard (as indicated by the number of commits and issues in the shell-quote repo!)

Webpack shouldn't really care about double quotes in particular, the OS' shell should itself be the one delimiting arguments. E.g. in your basic C program, your main function can take arguments; parsing those is not part of the C standard library or compiler, it's the OS.

What happens if you pass that whole array [--port, String(port), '--config', config, '--color'] through shell-quote?

@beescuit
Copy link
Author

Hi, I'm not sure that (in this case) the OS is parsing the parameters.
image
As you can see in the image, when I try to run webpack-dev-server --port 13370 --config 'C:\Users\Rafael\Desktop\teste com espacos\amazingly-incredible-bear\webpack.config.js' --color webpack tries to find the provided path in the CWD. (single-quoted)
When the path is double-quoted, it runs without any problem.
Also, the quote function from shell-quote returns a string and not an array because they join the array when returning the result.
Even if I try to put the array through shell-quote, which returns --port 13370 --config 'C:\\Users\\Rafael\\Desktop\\teste com espacos\\amazingly-incredible-bear\\webpack.config.js' --color, and run the dev server manually, it returns the same error. Error: Cannot find module 'C:\Users\Rafael\Desktop\cdk\'C:\Users\Rafael\Desktop\teste'

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants